TURBU Tech

What with recent events and all, it doesn’t look like Nick Hodges is going to get around to finishing up his Pretty Good Practices series anytime soon.  So I hope he doesn’t mind if I post one of my own.

Code Review is a very useful principle for keeping bugs out of the codebase.  At work we’ve got a rule which is (usually) followed pretty strictly: Any non-trivial change that might affect functionality needs to be reviewed by another developer before checkin.  Usually this is just a “rubber stamp” sort of thing.  You pull up a diff view against the source control version in Beyond Compare and show the other guy what you changed, he says “OK, looks good, go ahead and check it in,” and you go ahead and check it in.  But every once in a while they’ll catch something.

It can be just about anything, from debug code (commented out or left active) still in the code you wrote, to formatting problems that make it harder to read, to “I really think you could do this better if you were to change X and Y”, or even “that looks like it won’t work if the user does X.”  Occasionally the reviewer will even catch a syntax error that would have broken the build if you’d checked it in, either because you didn’t test it by compiling first or because you forgot to save your latest changes in the IDE.

It happens to everyone.  I’ve found all of these issues in my coworkers’ code, and had them find all of them in mine, at one point or another.  And when these sorts of things do get caught before going into the official codebase, it improves the quality of your program.  This is basically a small-scale version of Linus’s Law.

But a formal code review is not the only way to apply the principle.  You can catch a lot just by reviewing your own code, especially in a diff viewer like Beyond Compare.  Seeing what’s changed highlighted in red gives you a slightly different perspective than just looking at the ordinary code view, and sometimes it brings a mistake to your attention.  I usually check the diffs myself at work before asking a coworker for a review, and I always check diffs before checking any code in to the TURBU project repository.

Independent review can also be useful in actually finding the problem you’re trying to solve.  Maybe once or twice a month, I get a “hey Mason, can you come look at this code with me?  I need another pair of eyeballs” request from someone at work.  Sometimes I can help track it down, sometimes I can’t, and sometimes I just provide a useful suggestion.

I remember one time someone asked me to help track down a data corruption issue.  When I started looking at the code, it was a really ugly implementation of an array-based stack that someone else had written long ago, using pointers and GetMem/ReallocMem calls all over the place.  Looked like something straight out of my Data Structures class in college.  We’ve got a joke at work that a bad programmer can write C in any language, and that’s exactly what this looked like: someone writing C in Delphi.

At least there was a well-defined interface encapsulating this whole mess, so I said “I don’t think I can trace this very well, but it’s probably an out-of-bounds issue.  Any good reason why you couldn’t rewrite this with Delphi’s dynamic arrays and let Bounds Checking find the error for you?”  He did, and sure enough the problem became obvious real quickly.  It was trying to do a Pop on an empty stack at one point, and the debugger pointed out exactly where once the compiler was able to apply error checking to the problem.

Reviewing your code, and having someone else review it if possible, is a good way to catch bugs that might end up slipping into the codebase unnoticed otherwise, and it can also be helpful a lot of times to track down tricky errors if someone else is watching the debugger with you.  If you don’t do it in your development work, try it and see if it makes a difference.  It’s sorta like a seatbelt.  Most of the time it won’t do much for you, but when it does it’s very helpful.